Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix netconf 1.1 chunk parse #181

Merged
merged 3 commits into from
Apr 22, 2024

Conversation

netixx
Copy link
Contributor

@netixx netixx commented Apr 18, 2024

I stumbled upon an issue with the current netconf 1.1 parser.

My payload contained ######## characters on a single line, which matched the pattern defined in the regex erroneously.

I added a test for it, and proposed a new implementation of the chunk matching mechanism using a basic for loop.

Since the size of the chunk is known in advance, we only need to detect the chunk marker, and consume all data up to the length of the chunk and repeat.

As a bonus, we don't need to do expensive regex on very large payloads, so we should achieve faster speeds and less memory usage.

@netixx netixx changed the title Fix/netconf chunk parse Fix netconf 1.1 chunk parse Apr 18, 2024
@netixx netixx force-pushed the fix/netconf-chunk-parse branch 2 times, most recently from 4b78220 to ce53aba Compare April 18, 2024 15:57
@carlmontanari
Copy link
Contributor

ah very nice. I have some fear about this due to the newline shenangengins that it looks like you ran into as well :) will take a closer peak this weekend to test it out!

@carlmontanari
Copy link
Contributor

went through all that to make sure I understood it all! very nice solution @netixx ! I made some minor changes -- mostly just tweaking things so it fit my brain a bit better.

only meaningful change it think is that that the max chunk length check. I think we only need to check chunk lengths for up to 10 chars (that giant number from the rfc but the actual char positions in that number) -- so I changed things a bit to only check up to 10 chars. if the chunkSizeStr is empty we know we didn't find our new line and can bail out.

let me know if you're ok with these tweaks and then we can get it merged and cut another minor release!

@netixx
Copy link
Contributor Author

netixx commented Apr 22, 2024

It's all good to me, the way I was calculating the max chunk length was a bit silly indeed :)
I could test that it still works fine on my subset of devices so it ll good to go on my side!

@carlmontanari carlmontanari merged commit 2f77739 into scrapli:main Apr 22, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants